-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix IPv6 parsing errors in semconv.NetAttributesFromHTTPRequest #2285
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2285 +/- ##
=====================================
Coverage 72.6% 72.6%
=====================================
Files 172 172
Lines 11919 11898 -21
=====================================
- Hits 8661 8646 -15
+ Misses 3023 3019 -4
+ Partials 235 233 -2
|
Standardize order of checks for IP, Name, Port
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting the implementation to shrink given net.SplitHostPort
. Can we use it in the first line or two and take the fallback measures only if required—perhaps not at all? Under what circumstances would Go populate RemoteAddr
without a port?
i.e. assume net.SplitHostPort(input) will succeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@garthk I took your advice and refactored the code to assume the happy path succeeds, i.e. has a combination of both host and port. I do use the same routine to extract the hostIP/hostName and port for both In any case, it looks like all the new IPv6 tests are passing and there are 20 fewer lines of code in |
fixes #2283